Move the CreateTxKey and CreateTxRecord function into CatalogFactory to support multi table engines#111
Conversation
a260a2f to
1b6a1c8
Compare
WalkthroughConst-qualified and expanded MariaCatalogFactory API; removed Sequences special-case branches when creating table schemas and PK/CC maps. Eloq init now wires a local 3-slot Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Startup
participant MC as MariaCatalogFactory
participant CF as catalog_factory[3]
participant DS as DataStoreServiceClient
participant TX as TxService
Startup->>MC: ensure global `maria_catalog_factory`
Startup->>CF: assemble array { &MC, nullptr, nullptr }
Startup->>DS: construct DataStoreServiceClient(CF, ds_config, ds_srv)
Startup->>TX: construct TxService(..., CF, ...)
note right of MC #D3E4CD: MC exposes const APIs:\nCreateTxKey(), CreateTxKey(data,size),\nPackedNegativeInfinity(), CreateTxRecord()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
storage/eloq/eloq_catalog_factory.h (1)
229-233: Approve — sentinel is process-lifetime static; optional [[nodiscard]] + doc recommended.EloqKey::PackedNegativeInfinity() and the function-local static packed_negative_infinity_tx_key (returned pointer) are static locals, so the pointer has process lifetime and is non-owning (storage/eloq/eloq_key.h).
- txservice::TxKey CreateTxKey() const override; - txservice::TxKey CreateTxKey(const char *data, size_t size) const override; - const txservice::TxKey *PackedNegativeInfinity() const override; - std::unique_ptr<txservice::TxRecord> CreateTxRecord() const override; + [[nodiscard]] txservice::TxKey CreateTxKey() const override; + [[nodiscard]] txservice::TxKey CreateTxKey(const char *data, size_t size) const override; + // Not owned; process-lifetime sentinel. Never free. + [[nodiscard]] const txservice::TxKey *PackedNegativeInfinity() const override; + [[nodiscard]] std::unique_ptr<txservice::TxRecord> CreateTxRecord() const override;storage/eloq/ha_eloq.cc (2)
2396-2396: Potential lifetime bug: ensure the factory array outlives users or make it static.If DataStoreServiceClient/TxService retain the pointer-to-array (vs copying its contents), this stack-allocated array will dangle after eloq_init_func returns. Make it static (or ensure constructors copy into owned storage). Also consider centralizing the slot count instead of a magic 3.
Apply this minimal, safe change:
- CatalogFactory *catalog_factory[3]{&maria_catalog_factory, nullptr, nullptr}; + static CatalogFactory *catalog_factory[3]{&maria_catalog_factory, nullptr, nullptr};Follow‑ups:
- Confirm both DataStoreServiceClient and TxService copy the incoming pointers rather than keeping the raw pointer-to-array.
- If they do keep the pointer, keep this array static or pass an owned container (e.g., std::array) and have callees copy it.
2742-2743: Validate handling of null factory slots and const‑correctness.You pass two nullptr slots. Please confirm both DataStoreServiceClient and downstream consumers:
- iterate defensively (skip nullptrs),
- don’t assume a fixed non-null count,
- don’t dereference blindly.
If their APIs are read‑only, prefer taking
const CatalogFactory* const*to advertise const usage across the call chain.storage/eloq/eloq_catalog_factory.cpp (2)
725-728: Delegate to existing helpers to avoid duplication.Reuse EloqKey helpers to centralize construction logic.
txservice::TxKey MariaCatalogFactory::CreateTxKey() const { - return txservice::TxKey(std::make_unique<EloqKey>()); + return EloqKey::CreateDefault(); }
729-734: Same: prefer the helper factory for clarity and consistency.Reduces casting noise and keeps semantics in one place.
txservice::TxKey MariaCatalogFactory::CreateTxKey(const char *data, size_t size) const { - return txservice::TxKey( - std::make_unique<EloqKey>(reinterpret_cast<const uchar *>(data), size)); + return EloqKey::Create(data, size); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
storage/eloq/eloq_catalog_factory.cpp(3 hunks)storage/eloq/eloq_catalog_factory.h(1 hunks)storage/eloq/ha_eloq.cc(2 hunks)storage/eloq/log_service(1 hunks)storage/eloq/store_handler(1 hunks)storage/eloq/tx_service(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
storage/eloq/eloq_catalog_factory.h (2)
storage/eloq/eloq_catalog_factory.cpp (12)
NegativeInfKey(715-718)NegativeInfKey(715-715)PositiveInfKey(720-723)PositiveInfKey(720-720)CreateTxKey(725-728)CreateTxKey(725-725)CreateTxKey(729-734)CreateTxKey(729-730)PackedNegativeInfinity(735-738)PackedNegativeInfinity(735-735)CreateTxRecord(739-743)CreateTxRecord(740-740)storage/eloq/eloq_key.h (11)
TxKey(95-99)TxKey(101-104)TxKey(124-840)TxKey(126-129)TxKey(568-840)TxKey(570-572)TxKey(574-577)TxKey(579-599)TxKey(581-583)TxKey(585-588)PackedNegativeInfinity(117-122)
storage/eloq/eloq_catalog_factory.cpp (1)
storage/eloq/eloq_key.h (13)
TxKey(95-99)TxKey(101-104)TxKey(124-840)TxKey(126-129)TxKey(568-840)TxKey(570-572)TxKey(574-577)TxKey(579-599)TxKey(581-583)TxKey(585-588)NegativeInfinity(366-370)PositiveInfinity(372-376)PackedNegativeInfinity(117-122)
🔇 Additional comments (10)
storage/eloq/store_handler (1)
1-1: Manual verification required — submodule pointers & CatalogFactory API presenceAutomated sandbox check returned no output; unable to verify submodule SHAs or API/ABI alignment.
- Confirm storage/eloq/store_handler@a988f60 exposes CatalogFactory CreateTxKey/CreateTxRecord.
- Confirm storage/eloq/log_service and storage/eloq/tx_service submodule SHAs are compatible with the parent code.
- Confirm CI runs
git submodule update --init --recursive.Run locally to verify:
#!/bin/bash set -euo pipefail BASE=origin/eloq-10.6.10 git fetch origin eloq-10.6.10:refs/remotes/origin/eloq-10.6.10 echo "Submodule pointers (base -> PR):" for path in storage/eloq/store_handler storage/eloq/log_service storage/eloq/tx_service; do old_sha=$(git show "$BASE:$path" 2>/dev/null | awk '{print $3}') new_sha=$(awk '{print $3}' "$path" 2>/dev/null || true) printf "%-32s %s -> %s\n" "$path" "${old_sha:-<absent>}" "${new_sha:-<absent>}" done echo -e "\n.gitmodules entries for changed submodules:" for name in storage/eloq/store_handler storage/eloq/log_service storage/eloq/tx_service; do git config --file .gitmodules --get-regexp "submodule.${name}\.(path|url)" || true donestorage/eloq/log_service (1)
1-1: Submodule bump looks fine — confirm lockstep compatibility & ensure CI inits submodules.gitmodules contains the eloq submodules and git submodule status shows they’re pinned but mostly uninitialized here (eloq_metrics: 9ff8eddf..., log_service: 050631894d8a9fc098f2f162d8de38deea421509, store_handler: a988f60e76e3623b7fe4ecc344eadec4e56b2592); tx_service is initialized at 6e138befd2264268361e983fa968be42b60c619f (remotes/origin/create_tx_key). Ensure CI runs git submodule update --init --recursive and run integration tests to verify CatalogFactory/constructor-signature compatibility between log_service, store_handler and tx_service.
storage/eloq/tx_service (1)
1-1: Submodule bump verified — OK to approvestorage/eloq/tx_service is pinned to 6e138befd2264268361e983fa968be42b60c619f; nested submodules checked out at abseil-cpp @ 69195d5bd2416a7224416887c78353ee8edf67ee and tx-log-protos @ 3c3a1cf59a55f7a09e3efd129931234cb8e7d540; no local modifications detected.
storage/eloq/eloq_catalog_factory.h (1)
225-228: Const-qualifying sentinel key accessors is correct.Signatures match intent and improve const-correctness. No further issues spotted.
storage/eloq/eloq_catalog_factory.cpp (6)
715-718: Const‑correct NegativeInfKey — LGTM.Makes the API safer; behavior unchanged.
720-723: Const‑correct PositiveInfKey — LGTM.Consistent with the NegativeInf variant.
735-738: PackedNegativeInfinity exposure — LGTM.Stable static pointer; matches existing EloqKey API.
739-743: CreateTxRecord wiring — LGTM.Correct type erasure to TxRecord via unique_ptr.
572-576: Add a runtime engine check in CreateTableSchema (don’t rely on assert).assert(...) is compiled out in release; if MariaCatalogFactory::CreateTableSchema is ever invoked for a non‑EloqSql TableName you will silently construct a MysqlTableSchema. Either replace the assert with an explicit runtime guard that logs and returns/throws on mismatch, or ensure GetCatalogFactory(...) can never return MariaCatalogFactory for non‑EloqSql.
Location: storage/eloq/eloq_catalog_factory.cpp (MariaCatalogFactory::CreateTableSchema).
583-590: No change required — engine assert already present and callers are engine-specific.CreatePkCcMap contains assert(table_name.Engine() == txservice::TableEngine::EloqSql) and callers call GetCatalogFactory(table_name.Engine())->CreatePkCcMap(...), so non‑EloqSql flows should not reach this code (see storage/eloq/tx_service/src/cc/cc_shard.cpp around the CreatePkCcMap call sites).
e400305 to
d3f9016
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
storage/eloq/eloq_catalog_factory.cpp (4)
572-576: Prefer runtime guard over assert for engine mismatch (release safety).Asserts vanish in release builds; add a defensive check to fail fast instead of proceeding with a wrong engine.
- assert(table_name.Engine() == txservice::TableEngine::EloqSql); + if (unlikely(table_name.Engine() != txservice::TableEngine::EloqSql)) { + LOG(ERROR) << "MariaCatalogFactory expects EloqSql engine in CreateTableSchema"; + return nullptr; + }
583-590: Same engine guard for CreatePkCcMap.Mirror the runtime check to avoid constructing a map for an unsupported engine.
- assert(table_name.Engine() == txservice::TableEngine::EloqSql); + if (unlikely(table_name.Engine() != txservice::TableEngine::EloqSql)) { + LOG(ERROR) << "MariaCatalogFactory expects EloqSql engine in CreatePkCcMap"; + return nullptr; + }
715-723: Reuse TxKey singletons instead of constructing wrappers each call.Slightly cheaper and clearer; delegates to EloqKey helpers.
txservice::TxKey MariaCatalogFactory::NegativeInfKey() const { - return txservice::TxKey(EloqKey::NegativeInfinity()); + return EloqKey::GetNegInfTxKey(); } txservice::TxKey MariaCatalogFactory::PositiveInfKey() const { - return txservice::TxKey(EloqKey::PositiveInfinity()); + return EloqKey::GetPosInfTxKey(); }
725-734: Delegate TxKey creation to EloqKey::Create helpers (single source of truth).*Reduces duplication and keeps construction centralized.
txservice::TxKey MariaCatalogFactory::CreateTxKey() const { - return txservice::TxKey(std::make_unique<EloqKey>()); + return EloqKey::CreateDefault(); } txservice::TxKey MariaCatalogFactory::CreateTxKey(const char *data, size_t size) const { - return txservice::TxKey( - std::make_unique<EloqKey>(reinterpret_cast<const uchar *>(data), size)); + return EloqKey::Create(data, size); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
storage/eloq/eloq_catalog_factory.cpp(3 hunks)storage/eloq/eloq_catalog_factory.h(1 hunks)storage/eloq/ha_eloq.cc(2 hunks)storage/eloq/log_service(1 hunks)storage/eloq/store_handler(1 hunks)storage/eloq/tx_service(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- storage/eloq/tx_service
🚧 Files skipped from review as they are similar to previous changes (3)
- storage/eloq/log_service
- storage/eloq/store_handler
- storage/eloq/eloq_catalog_factory.h
🧰 Additional context used
🧬 Code graph analysis (1)
storage/eloq/eloq_catalog_factory.cpp (1)
storage/eloq/eloq_key.h (13)
TxKey(95-99)TxKey(101-104)TxKey(124-840)TxKey(126-129)TxKey(568-840)TxKey(570-572)TxKey(574-577)TxKey(579-599)TxKey(581-583)TxKey(585-588)NegativeInfinity(366-370)PositiveInfinity(372-376)PackedNegativeInfinity(117-122)
🔇 Additional comments (4)
storage/eloq/ha_eloq.cc (2)
2741-2743: Verify DataStoreServiceClient does not retain pointer to caller-owned arraystorage/eloq/ha_eloq.cc:2741-2743 — confirm the DataStoreServiceClient constructor copies the three CatalogFactory* pointers into its own internal storage (e.g., std::array/std::vector) rather than storing the incoming array address; if it currently retains the pointer, change it to copy ownership or document ownership semantics. Definition not found in repo — manual verification required.
2396-2396: Confirm null slots in catalog_factory are safe or initialize them.ha_eloq.cc:2396: CatalogFactory *catalog_factory[3]{&maria_catalog_factory, nullptr, nullptr};
This array is passed to EloqDS::DataStoreServiceClient (ha_eloq.cc:2741) and to TxService ctor (ha_eloq.cc:3200/3219). DataStoreServiceClient/TxService definitions are not present in this repo, so null-safety cannot be confirmed. Confirm those constructors only probe valid entries or (alternatively) populate the remaining slots (e.g., sequence/system catalogs) or change the API to accept a size/container.
storage/eloq/eloq_catalog_factory.cpp (2)
739-744: LGTM: CreateTxRecord returns EloqRecord via unique_ptr.Fits the new factory surface; no issues.
735-738: LGTM — PackedNegativeInfinity signature verifiedHeader storage/eloq/eloq_catalog_factory.h declares
const txservice::TxKey *PackedNegativeInfinity() const overrideand the implementation returnsEloqKey::PackedNegativeInfinityTxKey().
d3f9016 to
86ac654
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
storage/eloq/eloq_catalog_factory.cpp (4)
572-573: Add a release-safe guard for non-EloqSql engines.asserts vanish in release; add a runtime check to avoid misusing this factory with other engines.
- assert(table_name.Engine() == txservice::TableEngine::EloqSql); + assert(table_name.Engine() == txservice::TableEngine::EloqSql); + if (table_name.Engine() != txservice::TableEngine::EloqSql) { + return nullptr; + }
583-584: Same engine guard for CreatePkCcMap.Mirror the defensive check to prevent accidental use in release builds.
- assert(table_name.Engine() == txservice::TableEngine::EloqSql); + assert(table_name.Engine() == txservice::TableEngine::EloqSql); + if (table_name.Engine() != txservice::TableEngine::EloqSql) { + return nullptr; + }
725-728: Reuse existing EloqKey helper.Use EloqKey::CreateDefault() to centralize construction semantics.
- return txservice::TxKey(std::make_unique<EloqKey>()); + return EloqKey::CreateDefault();
729-734: Prefer EloqKey::Create for byte-backed keys.Reduces duplication and keeps behavior consistent with other call sites.
- return txservice::TxKey( - std::make_unique<EloqKey>(reinterpret_cast<const uchar *>(data), size)); + return EloqKey::Create(data, size);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
storage/eloq/eloq_catalog_factory.cpp(3 hunks)storage/eloq/eloq_catalog_factory.h(1 hunks)storage/eloq/ha_eloq.cc(2 hunks)storage/eloq/log_service(1 hunks)storage/eloq/store_handler(1 hunks)storage/eloq/tx_service(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- storage/eloq/tx_service
- storage/eloq/ha_eloq.cc
- storage/eloq/store_handler
- storage/eloq/log_service
🧰 Additional context used
🧬 Code graph analysis (2)
storage/eloq/eloq_catalog_factory.cpp (1)
storage/eloq/eloq_key.h (12)
TxKey(95-99)TxKey(101-104)TxKey(124-840)TxKey(126-129)TxKey(568-840)TxKey(570-572)TxKey(574-577)TxKey(579-599)TxKey(581-583)TxKey(585-588)NegativeInfinity(366-370)PackedNegativeInfinity(117-122)
storage/eloq/eloq_catalog_factory.h (2)
storage/eloq/eloq_catalog_factory.cpp (12)
NegativeInfKey(715-718)NegativeInfKey(715-715)PositiveInfKey(720-723)PositiveInfKey(720-720)CreateTxKey(725-728)CreateTxKey(725-725)CreateTxKey(729-734)CreateTxKey(729-730)PackedNegativeInfinity(735-738)PackedNegativeInfinity(735-735)CreateTxRecord(739-743)CreateTxRecord(740-740)storage/eloq/eloq_key.h (11)
TxKey(95-99)TxKey(101-104)TxKey(124-840)TxKey(126-129)TxKey(568-840)TxKey(570-572)TxKey(574-577)TxKey(579-599)TxKey(581-583)TxKey(585-588)PackedNegativeInfinity(117-122)
🔇 Additional comments (5)
storage/eloq/eloq_catalog_factory.h (1)
225-233: New TxKey/TxRecord API and const-correct infinities look good.Signatures align with the intended base interface, and const-qualification is appropriate.
storage/eloq/eloq_catalog_factory.cpp (4)
715-718: NegativeInfKey: LGTM.Returns a TxKey wrapping the static sentinel.
720-723: PositiveInfKey: LGTM.Consistent with NegativeInfKey.
735-738: PackedNegativeInfinity: LGTM.Returns the canonical static TxKey instance.
739-743: CreateTxRecord: LGTM.Allocation and return type are correct.
Change sequence table engine to InternalHash
86ac654 to
2728363
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
storage/eloq/eloq_catalog_factory.cpp (3)
715-723: Use existing helpers for +/-∞ TxKeys to reduce duplicationPrefer the canonical helpers for consistency.
txservice::TxKey MariaCatalogFactory::NegativeInfKey() const { - return txservice::TxKey(EloqKey::NegativeInfinity()); + return EloqKey::GetNegInfTxKey(); } txservice::TxKey MariaCatalogFactory::PositiveInfKey() const { - return txservice::TxKey(EloqKey::PositiveInfinity()); + return EloqKey::GetPosInfTxKey(); }
725-734: Reuse EloqKey::Create helpers for TxKey creation*Simplifies and centralizes construction.
txservice::TxKey MariaCatalogFactory::CreateTxKey() const { - return txservice::TxKey(std::make_unique<EloqKey>()); + return EloqKey::CreateDefault(); } txservice::TxKey MariaCatalogFactory::CreateTxKey(const char *data, size_t size) const { - return txservice::TxKey( - std::make_unique<EloqKey>(reinterpret_cast<const uchar *>(data), size)); + return EloqKey::Create(data, size); }
739-744: Prefer EloqRecord::Create() factoryMinor consistency improvement.
std::unique_ptr<txservice::TxRecord> MariaCatalogFactory::CreateTxRecord() const { - return std::make_unique<EloqRecord>(); + return EloqRecord::Create(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
storage/eloq/eloq_catalog_factory.cpp(3 hunks)storage/eloq/eloq_catalog_factory.h(1 hunks)storage/eloq/ha_eloq.cc(2 hunks)storage/eloq/log_service(1 hunks)storage/eloq/store_handler(1 hunks)storage/eloq/tx_service(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- storage/eloq/store_handler
- storage/eloq/eloq_catalog_factory.h
- storage/eloq/ha_eloq.cc
- storage/eloq/log_service
- storage/eloq/tx_service
🧰 Additional context used
🧬 Code graph analysis (1)
storage/eloq/eloq_catalog_factory.cpp (1)
storage/eloq/eloq_key.h (13)
TxKey(95-99)TxKey(101-104)TxKey(124-840)TxKey(126-129)TxKey(568-840)TxKey(570-572)TxKey(574-577)TxKey(579-599)TxKey(581-583)TxKey(585-588)NegativeInfinity(366-370)PositiveInfinity(372-376)PackedNegativeInfinity(117-122)
🔇 Additional comments (3)
storage/eloq/eloq_catalog_factory.cpp (3)
572-576: Assert on EloqSql engine in CreateTableSchema — looks right; please confirm callersGood guard for multi-engine setups. Please verify no non‑EloqSql table types route here after the wiring changes.
583-590: Assert on EloqSql engine in CreatePkCcMap — confirm multi‑engine pathLGTM. Ensure other engines have their own factory paths for PK CC map creation.
735-738: Packed -∞ exposure via EloqKey helper — LGTMConsistent with EloqKey::PackedNegativeInfinityTxKey().
2728363 to
1205a12
Compare
Summary by CodeRabbit
Refactor
Chores